lib: fix shared buffer growability validation - #65845
Conversation
Use the intrinsic growable getter instead of buffer.growable so shadowed properties cannot bypass validation or reject fixed buffers. Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: GitHub Copilot
panva
added
the
web-standards
nodejs-github-bot
added
c++
| { | ||
| const Local<Object> prototype = | ||
| SharedArrayBuffer::New(isolate, 0)->GetPrototypeV2().As<Object>(); | ||
| const Local<Object> descriptor = | ||
| prototype | ||
| ->GetOwnPropertyDescriptor( | ||
| context, FIXED_ONE_BYTE_STRING(isolate, "growable")) | ||
| .ToLocalChecked() | ||
| .As<Object>(); | ||
| const Local<Value> getter = | ||
| descriptor->Get(context, env->get_string()).ToLocalChecked(); | ||
| CHECK(getter->IsFunction()); | ||
| target | ||
| ->Set(context, | ||
| FIXED_ONE_BYTE_STRING(isolate, "getSharedArrayBufferGrowable"), | ||
| getter) | ||
| .Check(); | ||
| } |
There was a problem hiding this comment.
Is storing this and invoking with FunctionPrototypeCall definitely quicker than a fast binding that returns sab->GetBackingStore()->IsResizableByUserJavaScript()?
There was a problem hiding this comment.
I haven't benchmarked against a fast binding. Copilot tried that predicate first, but it wasn't equivalent. Shared Wasm memory can expose fixed and growable SAB wrappers over the same backing store.
node/deps/v8/src/objects/backing-store.h
Lines 142 to 146 in f16b556
The getter checks the individual buffer's growability, which is what Web IDL requires.
There was a problem hiding this comment.
Nice catch.
The flag check on the SAB itself would be sab.As<ArrayBuffer>()->IsResizableByUserJavaScript() – this checks the flag on the AB handle directly (https://github.com/v8/v8/blob/9f30cc5fc4ad6c3236c522fd23224ddc94591a20/src/api/api.cc#L4374-L4376). SAB handles are AB handles, so providing sab is known to be a SharedArrayBuffer, this should be directly equivalent to sab.growable.
Maybe worth a mini benchmark?
There was a problem hiding this comment.
Copilot ran a mini benchmark, about 10-24% faster for this check. But sab.As<ArrayBuffer>() fails with V8_ENABLE_CHECKS because the cast rejects shared buffers.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65845 +/- ##
==========================================
+ Coverage 90.17% 90.18% +0.01%
==========================================
Files 770 771 +1
Lines 264483 264633 +150
Branches 50262 50227 -35
==========================================
+ Hits 238491 238671 +180
+ Misses 16981 16958 -23
+ Partials 9011 9004 -7
🚀 New features to boost your workflow:
|
panva
added
author ready
github-actions
Bot
removed
the
request-ci
4 participants
Use the intrinsic growable getter instead of buffer.growable so shadowed properties cannot bypass validation or reject fixed buffers.
cc @nodejs/web-standards